Adding support for storm control on L2 interfaces and copy description on PC - #855
Conversation
juburnet
left a comment
There was a problem hiding this comment.
@juarocha - Please check if this scenario would be an issue:
Rule 312 iterates switch.get('interfaces') but VPC peer configurations live under vxlan.topology.switches[].vpc_peers[].interfaces (or similar path).
The VPC template now renders storm control for VPC interfaces. If a user sets both _percent and _pps on a VPC peer interface, Rule 312 would not catch it.
Confirm whether VPC peer data is in a different path that needs a second traversal.
juburnet
left a comment
There was a problem hiding this comment.
Approved.
- Consistent implementation across all 5+ interface templates — storm control blocks are identical in ndfc_interface_access.j2, ndfc_interface_access_po.j2, ndfc_interface_dot1q.j2, ndfc_interface_trunk.j2, ndfc_interface_trunk_po.j2, and ndfc_interface_vpc.j2.
- Defaults properly seeded — defaults.yml updated for all affected interface types (dot1q, access, trunk, access_po, trunk_po). New copy_description: false default added wherever copy_description was introduced.
- Validation rule 312 added — New 312_topology_interface_storm_control.py enforces the mutual exclusivity of percent vs. PPS. Good defensive validation.
- vPC rule 305 bug fix is a real fix — The old logic (len(interfaces["interfaces"]) == 2) was incorrect in multi-fabric scenarios. The new per-pair iteration logic correctly scopes the consistency check to actual vPC peers.
- data_model_key_check is self-contained — Rule 312 copies the utility method locally rather than inheriting from a base class inconsistently, matching the pattern of other rules in the codebase.
- All CI passes — Clean across all Python/Ansible matrix combinations.
dacasti2mx
left a comment
There was a problem hiding this comment.
Rule 312 assumes storm control is disabled whenever enable_storm_control is not explicitly repeated on the interface, but, this setting can also be inherited from defaults.vxlan
Since semantic validation runs before factory and custom defaults are merged, a valid interface that inherits enable_storm_control: true and only defines
its threshold is rejected.
Could Rule 312 validate the effective value using this precedence:
- explicit interface value, matching custom default, then factory default?
- It would also behelpful to add a test covering this inherited-default scenario.
For example, this should be valid because storm control is enabled through the access-interface defaults:
defaults:
vxlan:
topology:
switches:
interfaces:
topology_switch_access_interface:
enable_storm_control: truevxlan:
topology:
switches:
- name: leaf-1
interfaces:
- name: Ethernet1/1
mode: access
storm_control_broadcast_level_percent: 10in this example, Rule 312 sees no enable_storm_control directly under Ethernet1/1, assumes false, and rejects the threshold even though the effective value inherited
from defaults is true.
|
Rule logic fixed. since defaults are merged after the validation we can't use them on the rule. The only validation is that percent-based and pps-based settings are not mixed on the same interface |
|
@juarocha for Storm control, add ND 4.1 version requirement in templates as discussed. |
done |
dacasti2mx
left a comment
There was a problem hiding this comment.
live validation
Env: ND 4.1.1g / NDFC 12.4.1.321 · MCFG child fabric (FAB1) · targets NX-OSv (N9K-C9300v, 10.5(5))
Heads: validated live at 318b9cd; re-checked at 21d215e (delta = templates-only version gate, 0 rule changes)
Result: LGTM — features validated end-to-end; the "no config when not applicable" behaviors are by-design (see
Notes).
Negative / validation
| # | Model | Expected | Result |
|---|---|---|---|
| N1 | percent + pps on same intf | Rule 312 FAIL | ✅ 312 FAIL |
| N2 | enable:false + threshold |
accepted; threshold not applied (storm-control off) | ✅ nac-validate passes; render |
| omits it (by design) | |||
| N3 | vPC peers differ | Rule 305 FAIL | ✅ 305 FAIL |
| N4 | same vpc_id on 2 pairs |
PASS | ✅ PASS (pairing fix) |
Physical interface (subject Eth1/7, control Eth1/8 untouched)
| Stage | Change | Result (NX-OS) |
|---|---|---|
| L0 | access, no storm-control | ✅ access, no storm-control |
| L1 | enable + shutdown + bcast 5.00% | ✅ storm-control broadcast level 5.00 + action shutdown |
| L2 | rerun | ✅ idempotent |
| L3 | 20.00% + trap + mcast 10.00% | ✅ updated cleanly (no stale values) |
| L4 | percent → pps 1000 | ✅ storm-control broadcast level pps 1000 |
| L5 | disable | ✅ storm-control fully removed |
| L6 | trunk + 10.00% | ✅ trunk + storm-control |
| L7 | dot1q + 10.00% | ✅ mode dot1q-tunnel + storm-control |
| L8 | restore | ✅ clean |
| inherited | default enable=true, no explicit enable | ✅ threshold renders (inheritance works) |
Port-channels + copy_description
| Stage | Change | Result |
|---|---|---|
| P1 | access PO + 5% + copy_description:true |
✅ PO storm-control; description copied to members |
| P2 | verify | ✅ member desc == PO desc |
| P3 | update description | ✅ propagated to members |
| P4 | percent → pps | ✅ clean transition |
| P5 | copy_description:false |
✅ stops copying; previously-copied member desc retained (by design) |
| P6 | trunk PO + storm-control | ✅ (see Notes: NX-OS box-wide %/pps constraint) |
| P7 | routed PO + copy_description, no storm-control |
✅ copy_description works; storm-control correctly absent |
| P8 | removal | ✅ requires role_remove (merged create is additive) |
| P9 | rerun | ✅ idempotent |
vPC / ToR pair
| # | Case | Result |
|---|---|---|
| D1–D3/D5 | storm-control + copy_description on vPC PO, both peers | ✅ propagated to both peers + members; Rule 305 PASS |
| D4 | pps on vPC | ⏭️ by equivalence (pps validated in L4/P4) |
| D6 | update | ✅ propagated to both |
| D7 | removal | ✅ reverted cleanly |
| D8 | rerun | ✅ idempotent |
| D9 | peers differ | ✅ Rule 305 FAIL before mutation |
| D10 | percent + pps | ✅ Rule 312 FAIL |
| D11 | same vpc_id on 2 pairs |
✅ Rule 305 PASS (pairing fix) |
Related Issue(s)
Fixes #850
Fixes #853
Fixes #854
Fixes #857
Related Collection Role
Related Data Model Element
Proposed Changes
For storm control, adding new keys for under L2 interfaces:
For copy description, adding new keys for under port-channel interfaces:
Test Notes
Cisco Nexus Dashboard Version
4.1
Checklist